-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use Thread BR extended address in WS API calls #21172
Conversation
This follows the HA Core change and passes the extended address to the OTBR WS API calls where necessary. It also follows the new OTBR info format which potentially includes multiple OTBRs. This allows to support multiple OTBR managed by a single system. Note: There is one corner case when none of the OTBR is found via discovery. In this case we offer to reset the OTBR. Currently we simply offer this for the primary or first one found.
WalkthroughWalkthroughThe updates involve enhancements to the Changes
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Additional context usedBiome
Additional comments not posted (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Outside diff range comments (1)
src/panels/config/integrations/integration-panels/thread/thread-config-panel.ts (1)
Line range hint
463-481
: Specify a more accurate type thanany
forotbr
.Using
any
reduces type safety. Consider specifying a more accurate type for better type checking and maintainability.- const otbr = (ev.currentTarget as any).otbr as OTBRInfo; + const otbr = (ev.currentTarget as HTMLElement).otbr as OTBRInfo;Tools
Biome
[error] 472-472: Forbidden non-null assertion. (lint/style/noNonNullAssertion)
[error] 487-487: Unexpected any. Specify a different type. (lint/suspicious/noExplicitAny)
any disables many type checking rules. Its use should be avoided.
src/panels/config/integrations/integration-panels/thread/thread-config-panel.ts
Outdated
Show resolved
Hide resolved
src/panels/config/integrations/integration-panels/thread/thread-config-panel.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
src/panels/config/integrations/integration-panels/thread/thread-config-panel.ts
Outdated
Show resolved
Hide resolved
src/panels/config/integrations/integration-panels/thread/thread-config-panel.ts
Show resolved
Hide resolved
…d-config-panel.ts Co-authored-by: Bram Kragten <[email protected]>
…d-config-panel.ts Co-authored-by: Bram Kragten <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
Outside diff range comments (4)
src/panels/config/integrations/integration-panels/thread/thread-config-panel.ts (4)
Line range hint
491-504
: Improve error handling in_resetBorderRouter
.The method uses
any
for error handling, which reduces type safety. Consider specifying a more accurate type for the error and enhance error handling to manage specific error cases.try { await OTBRCreateNetwork(this.hass, otbr.extended_address); } catch (err: unknown) { showAlertDialog(this, { title: this.hass.localize("ui.panel.config.thread.otbr_config_failed"), text: (err as Error).message, }); // Log error or handle specific error cases here }Tools
Biome
[error] 472-472: Forbidden non-null assertion. (lint/style/noNonNullAssertion)
[error] 487-487: Unexpected any. Specify a different type. (lint/suspicious/noExplicitAny)
any disables many type checking rules. Its use should be avoided.
Line range hint
612-644
: Improve error handling in_changeChannel
.The method uses
any
for error handling, which reduces type safety. Consider specifying a more accurate type for the error and enhance error handling to manage specific error cases.try { const result = await OTBRSetChannel(this.hass, otbr.extended_address, channel); showAlertDialog(this, { title: this.hass.localize("ui.panel.config.thread.change_channel_initiated_title"), text: this.hass.localize("ui.panel.config.thread.change_channel_initiated_text", { delay: Math.floor(result.delay / 60) }), }); } catch (err: unknown) { if (err instanceof CustomError) { // Assume CustomError is a more specific error type you might expect showAlertDialog(this, { title: "Specific Error", text: err.message, }); } else { showAlertDialog(this, { title: "Error", text: (err as Error).message, }); } }
Line range hint
514-533
: Improve error handling in_setDataset
.The method uses
any
for error handling, which reduces type safety. Consider specifying a more accurate type for the error and enhance error handling to manage specific error cases.try { await OTBRSetNetwork(this.hass, otbr.extended_address, preferedDatasetId); } catch (err: unknown) { showAlertDialog(this, { title: this.hass.localize("ui.panel.config.thread.otbr_config_failed"), text: (err as Error).message, }); // Log error or handle specific error cases here }Tools
Biome
[error] 533-533: Unexpected any. Specify a different type. (lint/suspicious/noExplicitAny)
any disables many type checking rules. Its use should be avoided.
Line range hint
654-654
: Specify a more accurate type thanany
forev
.Using
any
reduces type safety. Consider specifying a more accurate type for better type checking and maintainability.- const dataset = (ev.currentTarget as any).networkDataset as ThreadDataSet; + const dataset = (ev.currentTarget as HTMLElement).networkDataset as ThreadDataSet;
src/panels/config/integrations/integration-panels/thread/thread-config-panel.ts
Show resolved
Hide resolved
src/panels/config/integrations/integration-panels/thread/thread-config-panel.ts
Show resolved
Hide resolved
src/panels/config/integrations/integration-panels/thread/thread-config-panel.ts
Show resolved
Hide resolved
src/panels/config/integrations/integration-panels/thread/thread-config-panel.ts
Outdated
Show resolved
Hide resolved
src/panels/config/integrations/integration-panels/thread/thread-config-panel.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Outside diff range comments (2)
src/panels/config/integrations/integration-panels/thread/thread-config-panel.ts (2)
Line range hint
211-254
: Complex logic in_renderNetwork
needs refactoring.The function
_renderNetwork
contains complex logic to determine the OTBR for a network. Consider refactoring to reduce complexity and improve maintainability.- const otbr = this._otbrInfo && this._otbrInfo[router.extended_address]; - const showOverflow = ("dataset" in network && router.border_agent_id) || otbr; + const otbr = this._otbrInfo?.[router.extended_address]; + const showOverflow = network.dataset && router.border_agent_id || otbr;Tools
Biome
[error] 212-212: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 210-210: Excessive complexity detected.
Please refactor this function to reduce its complexity score from 31 to the max allowed complexity 15.
(lint/complexity/noExcessiveCognitiveComplexity)
Line range hint
471-497
: Specify a more accurate type thanany
for event handling and improve error handling in border router methods.Using
any
reduces type safety. Consider specifying a more accurate type for better type checking and maintainability. Also, add specific error handling for the API call in_resetBorderRouter
.- const otbr = (ev.currentTarget as any).otbr as OTBRInfo; + const otbr = (ev.currentTarget as HTMLElement).otbr as OTBRInfo || null; if (!otbr) { return; } try { await OTBRCreateNetwork(this.hass, otbr.extended_address); } catch (err: any) { showAlertDialog(this, { title: this.hass.localize("ui.panel.config.thread.otbr_config_failed"), text: err.message, }); // Log error or handle specific error cases here }Tools
Biome
[error] 478-478: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 493-493: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
src/panels/config/integrations/integration-panels/thread/thread-config-panel.ts
Outdated
Show resolved
Hide resolved
src/panels/config/integrations/integration-panels/thread/thread-config-panel.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Bram Kragten <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
Outside diff range, codebase verification and nitpick comments (4)
src/panels/config/integrations/integration-panels/thread/thread-config-panel.ts (4)
Line range hint
211-254
: Refactor: Improve OTBR Handling in Router DisplayWhen displaying routers, the code retrieves OTBR information based on the router's extended address. This could be simplified and made safer with optional chaining.
- this._otbrInfo && this._otbrInfo[router.extended_address]; + this._otbrInfo?.[router.extended_address];This change ensures that the code does not break if
_otbrInfo
is undefined.Tools
Biome
[error] 212-212: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Line range hint
471-487
: Type Safety and Error Handling in Router Action HandlingThe
_handleRouterAction
method uses type assertions that can be improved for better type safety. Additionally, error handling could be enhanced.- const otbr = (ev.currentTarget as any).otbr as OTBRInfo; + const otbr = (ev.currentTarget as HTMLElement).otbr as OTBRInfo; if (!otbr) { return; }This change ensures that the method does not proceed if
otbr
is undefined, preventing potential runtime errors.
Line range hint
492-510
: Error Handling in Border Router ResetThe
_resetBorderRouter
method lacks specific error handling for the API call. Adding detailed error handling can improve user experience and debuggability.try { await OTBRCreateNetwork(this.hass, otbr.extended_address); } catch (err: any) { showAlertDialog(this, { title: this.hass.localize("ui.panel.config.thread.otbr_config_failed"), text: err.message, }); + // Log error or handle specific error cases here }
Adding comments or specific error handling strategies can help maintain and debug this method more effectively.
Line range hint
618-650
: Channel Change Validation and Error HandlingThe
_changeChannel
method allows for changing the communication channel of an OTBR. This method could benefit from additional validations and error handling to ensure the channel number is within the valid range before making the API call.const channel = parseInt(channelStr); + if (channel < 11 || channel > 26) { + showAlertDialog(this, { + title: this.hass.localize("ui.panel.config.thread.change_channel_invalid"), + text: this.hass.localize("ui.panel.config.thread.change_channel_range"), + }); + return; + } try { const result = await OTBRSetChannel(this.hass, otbr.extended_address, channel); showAlertDialog(this, { title: this.hass.localize("ui.panel.config.thread.change_channel_initiated_title"), text: this.hass.localize("ui.panel.config.thread.change_channel_initiated_text", { delay: Math.floor(result.delay / 60) }), }); } catch (err: any) { showAlertDialog(this, { title: "Error", text: err.message || err, }); }This change ensures that invalid channel numbers are caught early, preventing unnecessary API calls and potential errors.
Breaking change
Proposed change
This follows the HA Core change and passes the extended address to the OTBR WS API calls where necessary. It also follows the new OTBR info format which potentially includes multiple OTBRs.
This allows to support multiple OTBR managed by a single system.
Note: There is one corner case when none of the OTBR is found via discovery. In this case we offer to reset the OTBR. Currently we simply offer this for the primary or first one found.
Related Core PR: home-assistant/core#108282
Type of change
Example configuration
Additional information
Checklist
If user exposed functionality or configuration variables are added/changed:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation